-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
전남대 FE_이도현_2주차 과제 #19
base: leedyun
Are you sure you want to change the base?
Conversation
|
||
import React from 'react'; | ||
|
||
const Footer: React.FC = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포넌트 스타일링은 따로 안하셨을까요 ?
src/components/GiftRanking.tsx
Outdated
} | ||
|
||
const GiftRanking: React.FC = () => { | ||
const [filter, setFilter] = useState<string>('all'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter
타입이 정해져있는데 string 보다는 type 으로 지정하는게 type narrowing 측면에서 낫습니다.
src/components/GiftRanking.tsx
Outdated
|
||
const GiftRanking: React.FC = () => { | ||
const [filter, setFilter] = useState<string>('all'); | ||
const [subFilter, setSubFilter] = useState<string>('wanted'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subFilter
의 타입도 마찬가지 입니다.
src/components/GiftRanking.tsx
Outdated
{['all', 'women', 'men', 'teenagers'].map((category) => ( | ||
<div key={category} className={`button-container ${filter === category ? 'active' : ''}`} onClick={() => handleFilterChange(category)}> | ||
<button> | ||
<span>{category === 'all' ? 'ALL' : category === 'women' ? '👩🏻🦳' : category === 'men' ? '👨🏻🦳' : '👦🏻'}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 삼항 연산자는 가독성을 떨어뜨립니다.
객체로 이를 대체할 수 있습니다.
// e.g.
const CATEGORY = {
all: 'ALL',
women: '👩🏻🦳',
men: '👨🏻🦳',
};
<span>{CATEGORY[category] || '👦🏻'}</span>
src/components/GiftRanking.tsx
Outdated
<button> | ||
<span>{category === 'all' ? 'ALL' : category === 'women' ? '👩🏻🦳' : category === 'men' ? '👨🏻🦳' : '👦🏻'}</span> | ||
</button> | ||
<span className="category">{category === 'all' ? '전체' : category === 'women' ? '여성이' : category === 'men' ? '남성이' : '청소년이'}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 마찬가지 입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백해주셔서 감사합니다.
말씀하신 부분은 수정 완료했습니다.
GiftRanking 이외에 부분은 따로 수정할 부분은 없을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백해주셔서 감사합니다. 말씀하신 부분은 수정 완료했습니다. GiftRanking 이외에 부분은 따로 수정할 부분은 없을까요?
추가로 남겨드린 피드백만 반영해주시면 따로 없을거 같아요 !
refactor : filter와 subFilter 타입을 string에서 타입으로 변경 refactor : 카테고리와 서브카테고리 라벨을 객체로 대체
|
||
return ( | ||
<> | ||
<Header /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Header, Footer 등 반복되는 컴포넌트는 Layout 으로 구성할 수 있을거 같습니다.
아래 참고 하셔서 Layout 컴포넌트로 구성해보실 수 있을거 같아요.
ref. https://reactrouter.com/en/main/components/outlet
); | ||
}; | ||
|
||
export default App; | ||
const ApplicationRoutes = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Routes 는 별도의 컴포넌트로 작성해서 App.tsx 에서 import 하도록 변경하는게 가독성 측명에서 더 나을거 같습니다.
base repository 변경을 안해서 다시보냅니다!